Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Histogram binwidth #789

Merged
merged 6 commits into from
Aug 16, 2023
Merged

Histogram binwidth #789

merged 6 commits into from
Aug 16, 2023

Conversation

bbeat2782
Copy link

@bbeat2782 bbeat2782 commented Aug 9, 2023

Describe your changes

Add --binwidth/-W to ggplot histogram

Issue number

Closes #784

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--789.org.readthedocs.build/en/789/

fix

fix

fix

test functions

fix

fix

fix

fix

testing errors

add integration tests

fix

fix

fix

fix

fix
@bbeat2782 bbeat2782 marked this pull request as ready for review August 13, 2023 01:17
@bbeat2782
Copy link
Author

binwidth_demonstration.zip: contains comparisons with ggplot in R and error messages when invalid arguments are passed

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
doc/api/magic-plot.md Outdated Show resolved Hide resolved
@neelasha23
Copy link

neelasha23 commented Aug 14, 2023

  1. This looks incorrect (covering negative x values also)? What should be the expected behaviour if binwidth specified is larger than actual range of values?
Screenshot 2023-08-14 at 6 25 06 PM
  1. I think this should throw error? It's not clear which value of binwidth is considered
Screenshot 2023-08-14 at 6 40 18 PM

src/sql/plot.py Outdated Show resolved Hide resolved
src/sql/plot.py Outdated Show resolved Hide resolved
@bbeat2782
Copy link
Author

  1. This looks incorrect (covering negative x values also)? What should be the expected behaviour if binwidth specified is larger than actual range of values?

@neelasha23 when I check it with ggplot in R, it also covers negative x values. So instead of raising an error, it now displays a message that says Specified binwith {num} is larger than the range {num}. Please choose a smaller binwith. This message appears when the specified binwidth is strictly larger than the range.

fixed

  1. I think this should throw error? It's not clear which value of binwidth is considered

For this one, I tried running %sqlplot histogram --table diamonds.csv --table penguins.csv --column body_mass_g, where there are 2 --table arguments. But it doesn't throw an error. Instead, the code uses the latter argument. So I think solving this should be a separate issue. What do you think?

@bbeat2782 bbeat2782 requested a review from neelasha23 August 15, 2023 01:13
@bbeat2782 bbeat2782 requested a review from neelasha23 August 16, 2023 00:10
@neelasha23
Copy link

neelasha23 commented Aug 16, 2023

For this one, I tried running %sqlplot histogram --table diamonds.csv --table penguins.csv --column body_mass_g, where there are 2 --table arguments. But it doesn't throw an error. Instead, the code uses the latter argument. So I think solving this should be a separate issue. What do you think?

yes should be handled in separate issue

@edublancas edublancas merged commit 0e190b5 into ploomber:master Aug 16, 2023
@edublancas
Copy link

great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add binwidth to histogram
3 participants